-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Zoom Out: Remove unlock() from getSettings #64141
Conversation
…ly locked associated with the useZoomOut hook and feature.
Size Change: -10 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Flaky tests detected in f5e4bf6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10182256602
|
I think it is the right thing to do. I also did similar in this PR - #64073. I didn't expect there would be more instances, so didn't check. 😄 From what I know, there is a kind of private setting, though there are only two keys specified (neither are gutenberg/packages/block-editor/src/store/private-actions.js Lines 14 to 58 in ff59ffa
It also looks like the settings aren't locked, just that it might strip out the private keys. |
I think it might be incorrectly locked. gutenberg/packages/editor/src/components/provider/use-block-editor-settings.js Lines 329 to 331 in 30c6dfb
P.S. It seems that we have a couple of methods for making object value access private; we should probably settle on one. |
That's interesting. I wonder why the errors happen. I guess I should revert my PR, as it might have caused some issues. I also wonder why this is an editor setting compared to a private selector. It looks like the section client id is already derived from store state: gutenberg/packages/editor/src/components/provider/use-block-editor-settings.js Lines 143 to 154 in 30c6dfb
|
I'm unsure about that, but it seems a good candidate for the private selector. Maybe @ajlende or @scruffian can clarify the intention behind making it a setting. |
I'm also concerned about how this locking works when the editor settings are updated via the reducer as it creates a new object (this is where I was looking for signs of locking): gutenberg/packages/block-editor/src/store/reducer.js Lines 1653 to 1677 in 30c6dfb
|
@tjcafferkey, I think we might want to change how this setting is exposed. We should probably avoid adding it as a setting and create a new private selector that derives But let's see what others have to say. |
I think this does work ok as the locking data is reference via a key/value to the data, so it'll be retained in the new object. #64073 was reverted btw. @tjcafferkey Do you have more info about how to repro the warnings/errors you're seeing? One possible issue is that the locking of settings happens in the |
I believe this happens when you use the To provide testing instructions I'd likely have to use Edit: Apologies for the late reply here, was tied up with other things this week! |
I've been AFK for a (long) while so sorry not to have inputted here sooner.
My understanding (cc @scruffian and @MaggieCabrera who were here when I was away) is that The Site Editor uses the The Based on the above I don't think we can move the As to the locking element - I can't say, but I assume folks were trying to keep it private for now. |
Could you have a private action for setting the |
@talldan Sorry I realised I didn't fully address your point. The reason we can't put that logic directly in block editor, despite it being derived for interrogating the blocks in the editor, is that some of these blocks are specific to the WP editors ( |
Yes I was thinking about that just now. I think technically it's possible. I'm just wondering if we chose the settings root for a reason or whether it was just that alternatives weren't considered. I'm thinking about the benefits of that approach:
Is there anything else I'm missing? I'll have a go at implementing this approach in a separate PR. |
Some initial work here #64544 |
This PR is currently a WIP. I'm not sure if removing
unlock
is the correct thing to do as of yet.What?
Remove
unlock()
around the usage ofgetSettings()
Why?
This causes an error since it wasn't locked and surfaces when using the
useZoomOut
hook.How?
Remove
unlock()
from the necessary parts of the codebase associated with the error.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast